OCPBUGS-78995: Azure Machines: Accept explicit image ID#10712
Conversation
openshift-ci-robot
added
jira/severity-critical
|
@2uasimojo: This pull request references Jira Issue OCPBUGS-78995, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughAzure image handling now preserves explicit subscription resource paths and prevents a slicing panic when image IDs lack the expected resource-group segment. ChangesAzure image handling
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/asset/machines/azure/azuremachines.go`:
- Around line 402-404: Add a unit test covering the image-selection logic in the
function containing the rhcosImg branch: verify explicit managed or gallery
image IDs populate capz.Image.ResourceID, while marketplace URNs continue
populating the marketplace-specific fields. Include the explicit non-marketplace
ID case and assert the returned image values to prevent regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 882480ed-f9c2-442f-a701-307bffddd62a
📒 Files selected for processing (1)
pkg/asset/machines/azure/azuremachines.go
| case rhcosImg != "": | ||
| // An explicit non-marketplace URN was supplied. Use it as is. | ||
| return &capz.Image{ID: &rhcosImg} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a regression test for explicit image IDs.
Please add or point to a unit test verifying that managed/gallery image IDs become ResourceID, while marketplace URNs still populate marketplace fields. This branch changes the image-selection contract and should guard against regressions.
As per coding guidelines, all Go code under pkg/ must have unit tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/asset/machines/azure/azuremachines.go` around lines 402 - 404, Add a unit
test covering the image-selection logic in the function containing the rhcosImg
branch: verify explicit managed or gallery image IDs populate
capz.Image.ResourceID, while marketplace URNs continue populating the
marketplace-specific fields. Include the explicit non-marketplace ID case and
assert the returned image values to prevent regressions.
Source: Coding guidelines
cf09259 to
f2e7e73
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/asset/machines/azure/azuremachines.go`:
- Around line 402-404: Update the explicit resource-path branch in the rhcos
image handling to validate rhcosImg against an allow-list for a complete Azure
resource ID, including the subscription and /resourceGroups/ segments, before
constructing capz.Image. Reject malformed values such as /subscriptions/foo
safely, or ensure trimSubscriptionPrefix handles invalid input without slicing
at a negative index.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9d65d4e3-5ab5-4bb8-b72a-17c68750cfd9
📒 Files selected for processing (1)
pkg/asset/machines/azure/azuremachines.go
Machine API operator is limited in its ability to discover an appropriate default image for Machine[Set]s where it is not specified. Under direct cluster management, the workaround is to specify it. This change facilitates doing that when under remote/fleet management, such as via hive, via the `MachineSets()` function. It was already possible to supply an explicit *marketplace* image, but the paths for managed and gallery images were written with hardcoded path formats dependent on knowing things about architecture and HyperVGeneration, intended to be used day 0 by the installer itself, where the environment is controlled/known and additional discovery and validation is possible. With this change, we add a path for `MachineSets()` to accept an arbitrary OS image resource identifier, which may represent a managed or gallery image. This is one part of a two-part solution that will provide hive users an escape hatch if they encounter this MAO defaulting limitation in environments where marketplace images are not available/appropriate.
f2e7e73 to
2a1abcb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/asset/machines/azure/machines.go`:
- Around line 461-464: Add regression tests for the image-ID helper surrounding
the rgIndex validation, covering a valid subscription-scoped ID, an ID missing
“/resourceGroups/”, and empty input; assert the expected image result and ensure
malformed inputs do not panic. Place the tests with the existing unit tests for
the Azure machine helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5a6bfbb2-d7d7-4ebd-a72c-e7cdeb1d60c4
📒 Files selected for processing (2)
pkg/asset/machines/azure/azuremachines.gopkg/asset/machines/azure/machines.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/asset/machines/azure/azuremachines.go
| // Allow invalid inputs to fail downstream | ||
| if rgIndex < 0 { | ||
| return image | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add regression tests for valid and malformed image IDs.
This change fixes a panic path, but the modified helper has no visible unit tests. Cover a valid subscription-scoped ID, an ID without /resourceGroups/, and an empty input.
As per coding guidelines, all Go code under pkg/ must have unit tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/asset/machines/azure/machines.go` around lines 461 - 464, Add regression
tests for the image-ID helper surrounding the rgIndex validation, covering a
valid subscription-scoped ID, an ID missing “/resourceGroups/”, and empty input;
assert the expected image result and ensure malformed inputs do not panic. Place
the tests with the existing unit tests for the Azure machine helpers.
Source: Coding guidelines
There was a problem hiding this comment.
Can you point to an existing unit test that covers this function?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== Azure machine-related Go tests =='
fd -t f -e go . pkg/asset/machines/azure | sort
echo
echo '== Direct trimSubscriptionPrefix calls =='
rg -n -C 3 '\btrimSubscriptionPrefix\s*\(' pkg/asset/machines/azure -g '*_test.go' || true
echo
echo '== mapiImage calls in tests (indirect coverage candidate) =='
rg -n -C 3 '\bmapiImage\s*\(' pkg/asset/machines/azure -g '*_test.go' || true
echo
echo '== Image ResourceID assertions in Azure machine tests =='
rg -n -C 3 '(ResourceID|resourceGroups)' pkg/asset/machines/azure -g '*_test.go' || trueLength of output: 1037
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== Direct helper coverage in all Go tests =='
rg -n -C 3 '\btrimSubscriptionPrefix\s*\(' -g '*_test.go' . || true
echo
echo '== Indirect mapiImage coverage in all Go tests =='
rg -n -C 3 '\bmapiImage\s*\(' -g '*_test.go' . || true
echo
echo '== Tests covering the package or Azure resource-group image IDs =='
rg -n -C 3 '(pkg/asset/machines/azure|/resourceGroups/|ResourceID)' -g '*_test.go' . || trueLength of output: 3942
@2uasimojo, no—there is no existing unit test covering trimSubscriptionPrefix, directly or through mapiImage.
pkg/asset/machines/azure/ currently has no *_test.go files, and a repository-wide search found no test invocation of either helper. The closest match is pkg/types/azure/platform_test.go:TestSetBaseDomain, but it tests unrelated DNS zone parsing.
My original comment should have stated this more clearly: the requested tests would be new regression coverage, not an extension of an existing helper test.
There was a problem hiding this comment.
Cool, imma leave it alone then, unless a human feels otherwise.
There was a problem hiding this comment.
@2uasimojo, acknowledged. The panic fix is appropriately minimal; additional regression coverage can be added later if a human reviewer requests it.
There was a problem hiding this comment.
What is the motivation for this (I.e. what is the input/case where we hit this) change?
Generally the image function seems like a really good case for unit tests and should be easy to add with claude
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
/test azure-ovn-marketplace-images |
|
@2uasimojo: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Machine API operator is limited in its ability to discover an appropriate default image for Machine[Set]s where it is not specified. Under direct cluster management, the workaround is to specify it. This change facilitates doing that when under remote/fleet management, such as via hive, via the
MachineSets()function.It was already possible to supply an explicit marketplace image, but the paths for managed and gallery images were written with hardcoded path formats dependent on knowing things about architecture and HyperVGeneration, intended to be used day 0 by the installer itself, where the environment is controlled/known and additional discovery and validation is possible.
With this change, we add a path for
MachineSets()to accept an arbitrary OS image resource identifier, which may represent a managed or gallery image. This is one part of a two-part solution that will provide hive users an escape hatch if they encounter this MAO defaulting limitation in environments where marketplace images are not available/appropriate.Summary by CodeRabbit